Remove go-kit from being used in the core code
#20
Conversation
This commit removes usage of `go-kit` from the core code. We still allow a `go-kit` logger to be injected for backwards compatibility purposes. One behavioral change is introduced, resulting in the logging calls not setting the log level directly. If one needs the old behavior, they should initialize the logger like this: `logger = level.NewInjector(logger, level.DebugValue())` This will ensure the logger will add a `level=debug` attribute for the log lines where those were removed.
hslatman
force-pushed
the
herman/remove-go-kit
branch
from
f542092
to
a626337
Compare
|
Part of the motivation here was to remove the dependency completely (to have it removed from the go.sum). But I see the package is still used (in the test). Can it be removed completely? |
|
@jessepeterson what is the goal of fully removing it from |
|
Downstream dependencies also include references of their dependents, don't they? But more to the point if the dependency is still there.. why even remove it at all? |
|
Well, we got rid of
|
Yep, that's great! But here's an example project that tries to include it: https://gist.github.com/jessepeterson/671b3b7fe2140dda535dbb30166f33c4 As you can see from the
Yeah by moving away from go-kit logging to our own logging package there is no guarantee it'll work. I think that's implied. We now guarantee that we work with our logging interface (in
Yes, micromdm/scep is heavily invested in Go-kit in general but the plan is to move away from it. Who knows when that'll come—perhaps when when we drop server capability. The Nano* projects have been using jessepeterson/go-log (embedded) for some time but have recently switched to importing micromdm/nanolib/log which is the same, just imported. We'll write an adapter to slog at some point, too. |
|
@jessepeterson I've opened #21. Given that implementations should adhere to the Not sure why we got some of the new lines in |
Thanks! I'll also just add, when the eventual refactor comes about (I know, when we all have free time, right?), I would question even including a logger facility at all. This package is largely for data structures and marshaling to and fro. I'd prefer to have ways to access (or be able to export) the relevant logged data and push logging up to the package/library users. This was also brought up (forever ago) in the previous project over in micromdm/scep#62. I just thought removing the go-kit dependency (for now) was a step along the way, considering we have a local interface defined. Fwiw the logging could be removed now and keep the API unchanged (just not using any provided logger). ;P |
Yeah, I mostly agree. We should make use of some more sentinel errors or typed errors with appropriate context. And we could look into adding a method to get a
😅 |
Replaces #19 and #11.
This PR removes usage of
go-kitfrom the core code. We stillallow a
go-kitlogger to be injected for backwards compatibilitypurposes.
One behavioral change is introduced, coming down to the logging
calls not setting the log level directly. If one needs the old
behavior, they should initialize the logger like this:
logger = level.NewInjector(logger, level.DebugValue())This will ensure the logger will add a
level=debugattribute forthe log lines where those were removed. An example of this, and
verification can be found in the tests.
IMO this shouldn't cause too much trouble for people relying on it,
so I think it's OK to make this backwards incompatible behavior
change without cutting a
v2. In av2we would likely uselog/slog.@jessepeterson thanks for the nudge. The
go-kitdependencywas a thorn in the eye 😅 The tests didn't take into account the
actual log output, so that's why they succeeded with your changes.
I've added additional verification logic for the logs. That means
we still rely on
go-kitin the tests, but for dependents of the code,that shouldn't be an issue.